Skip to content

Addresses comments from #623 in ActiveRolesProvider - #708

Closed
collado-mike wants to merge 1 commit into
apache:mainfrom
collado-mike:mcollado-fix-securityctx
Closed

collado-mike wants to merge 1 commit into
apache:mainfrom
collado-mike:mcollado-fix-securityctx

Conversation

@collado-mike

Copy link
Copy Markdown
Contributor

Addresses comments from #623

public SecurityContext createSecurityContext(
SecurityContext ctx, AuthenticatedPolarisPrincipal principal) {
Set<String> validRoleNames = activeRolesProvider.getActiveRoles(principal);
if (validRoleNames.isEmpty()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making the tests fail, and I think we do have a problem. DefaultActiveRolesProvider expects the principal to have the role PRINCIPAL_ROLE_ALL when all roles are activated:

boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL);

But BasePolarisAuhtenticator creates a principal with an empty roles set in that case. You need to change BasePolarisAuthenticator.getPrincipal():

  if (tokenInfo.getScope() != null) {
      if (tokenInfo.getScope().equals(PRINCIPAL_ROLE_ALL)) {
        activatedPrincipalRoles.add(PRINCIPAL_ROLE_ALL); // this is missing
      } else {
        activatedPrincipalRoles.addAll(...);
      }
    }

@adutra

adutra commented Jan 14, 2025

Copy link
Copy Markdown
Contributor

@collado-mike if you want to rebase this PR, the two filters were moved/renamed to:

org.apache.polaris.service.auth.PolarisPrincipalAuthenticatorFilter
org.apache.polaris.service.auth.PolarisPrincipalRolesProviderFilter

Both are in polaris-service-common.

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions Bot added the stale label Feb 14, 2025
@github-actions github-actions Bot closed this Feb 27, 2025
@github-project-automation github-project-automation Bot moved this from PRs In Progress to Done in Basic Kanban Board Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants